fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553
fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553aaronjmars wants to merge 1 commit into
Conversation
|
Friendly bump @obra — small Host-header validation fix to close the DNS-rebinding window on brainstorm-server. Happy to address any feedback when you have a moment. |
|
Friendly second nudge @obra — small Host-header validation in |
|
Review note after reading the PR body and diff: The security issue this addresses looks real, and Host allowlisting is the right axis for DNS rebinding on the HTTP A couple of review blockers / cleanup items remain before this is easy to merge:
I did not find a correctness issue in the allowlist logic from the patch alone. The one thing I would re-run after rebasing to |
Allowlist the HTTP `Host` header on both the request path and the
WebSocket upgrade. Default allowlist covers `localhost`, `127.0.0.1`,
`[::1]` (with and without :PORT), plus the configured `BRAINSTORM_HOST`
and `BRAINSTORM_URL_HOST`. Operators can extend with
`BRAINSTORM_ALLOWED_HOSTS` (comma-separated) for tunneled setups.
Without this gate, a page on another origin can DNS-rebind its own
hostname to 127.0.0.1, hit `/` or `/files/*` on the running brainstorm
companion server, and read the active screen + content files even
though the listener is loopback-only. The same rebind would also let
the page complete a WebSocket upgrade and inject `{choice: ...}`
events into `state_dir/events`, which the agent reads as the user's
selection.
PR obra#1110 / issue obra#1014 already cover the WebSocket Origin axis. This
PR adds the complementary Host axis, which is the canonical defense
against DNS rebinding (Origin alone doesn't cover `/files/*`).
Tests: 6 new cases in tests/brainstorm-server/server.test.js cover
loopback accept, foreign-Host 421, /files/* foreign-Host 421, and a
forged-Host WebSocket upgrade. Existing 25 tests still pass (31/31).
Detected by: Aeon (https://github.com/aaronjmars/aeon-aaron) +
manual review against the brainstorm-server attack surface.
Severity: medium (cross-origin info disclosure + agent input
injection via DNS rebinding).
CWE-346 (Origin Validation Error), CWE-350 (Reliance on Reverse DNS
Resolution for a Security-Critical Action).
9b59c01 to
5b018e0
Compare
|
Thanks for the careful review @YOMXXX — addressed all three:
And agreed on the scope distinction you raised: this is HTTP+WS |
What problem are you trying to solve?
The brainstorm companion server (
skills/brainstorming/scripts/server.cjs) accepts HTTP and WebSocket connections without validating theHostheader. Combined with the listener's binding to127.0.0.1, that's enough to keep the TCP socket "local" — but it does not stop a DNS-rebinding attack:ws://127.0.0.1:<port>and the user opens the printed URL in their browser.https://attacker.example, which has its own DNS server return its hostname pointing first to its own IP, then re-resolves it to127.0.0.1after the page loads (DNS rebind).attacker.exampleissuesfetch('http://attacker.example:<port>/')andfetch('/files/...')— the TCP connection now lands on the loopback brainstorm server, but the browser sendsHost: attacker.exampleand treats the response as same-origin./files/*content. The attacker reads brainstorm screens and any file the agent dropped inCONTENT_DIR.{type: "click", choice: "<attacker text>"}send, which the server appends tostate_dir/events. On the next turn the agent reads that file as the user's selection.Issue #1014 describes the WebSocket-side variant of this; PR #1110 covers the WebSocket
Originaxis but leaves both HTTP info-disclosure paths and theHost-axis defense on WS in place. This PR closes theHost-header gap.Reproduction loop (without the fix), using the existing test harness as the "attacker":
What does this PR change?
Adds a single
Host-header allowlist (buildAllowedHosts()+isHostAllowed()) inserver.cjsand applies it before any work inhandleRequest()(returns421 Misdirected Request) and before the101handshake inhandleUpgrade()(destroys the socket). Default allowlist:localhost,127.0.0.1,[::1](bare and with:PORT) plus the configuredBRAINSTORM_HOST/BRAINSTORM_URL_HOST. Operators can extend viaBRAINSTORM_ALLOWED_HOSTS(comma-separated) for tunneled / containerized setups that already exist invisual-companion.md.Is this change appropriate for the core library?
Yes —
skills/brainstorming/scripts/server.cjsis a first-party companion server shipped with core. DNS-rebinding is not a project-specific concern; every brainstorm session running anywhere is exposed without this gate. The fix introduces no new dependencies, no third-party integrations, and no configuration the user is forced to set (defaults work in every documented setup).What alternatives did you consider?
start-server.sh,visual-companion.md,helper.js, the URL the agent prints, and how the user opens the page. PR harden brainstorming session access and cleanup boundaries #685 was bundled with permissions/cleanup/lockfile concerns and got closed quickly. A token approach is also strictly defense-in-depth on top of the Host check — DNS rebinding is solved by checking Host, regardless of token presence./and/files/*info-disclosure open. HTTP same-origin requests under DNS rebind don't sendOrigin, so anOrigincheck on the HTTP path would silently pass attacker traffic. Host validation is the canonical answer for the HTTP axis./files/*entirely: removes one disclosure path but not the other (/still serves the current screen). Also a much more invasive behavioral change.The Host allowlist is the minimum surgical fix that closes the rebinding surface end-to-end on both HTTP and WS paths.
Does this PR contain multiple unrelated changes?
No. Single concern: validate the
Hostheader against an allowlist before processing any HTTP request or completing a WebSocket upgrade. Two files touched — the server (+59) and the test file (+76) — both load-bearing for the same change.Existing PRs
Origincheck). This PR is intentionally complementary, not duplicative —HostandOriginare different headers covering different threat directions. PR harden brainstorming session access and cleanup boundaries #685 (closed) bundled a much broader token-auth + cleanup change; the maintainer's quick close on that one motivated the narrow scope here.Environment tested
New harness support (required if this PR adds a new harness)
N/A — no new harness. The existing
using-superpowersbootstrap is untouched.Evaluation
obra/superpowersfrom today's GitHub trending list (No marketplace.json? #3, ★191,721). After semgrep / trufflehog / osv-scanner came back clean, manual review of the brainstorm companion server surfaced the missing Host gate.cd tests/brainstorm-server && npm install && npm test. Both: 31 passed, 0 failed.GET / Host: evil.example:<port>→200+ brainstorm screen body returned.GET /files/<any> Host: evil.example→200/404depending on file.ws://evil.example:<port>via DNS-rebind hop → upgrade succeeds, server accepts{choice: "..."}and writes it tostate/events.421/421/ socket destroy with no101. LegitimateHost: localhost:<port>,127.0.0.1:<port>, and bare-loopback Hosts continue to return200and complete the WS handshake.Adversarial cases the new tests cover: foreign-Host on
/, foreign-Host on/files/*, and a forged-Host WebSocket upgrade routed back to the loopback listener via a custom DNSlookup.Rigor
superpowers:writing-skillsand completed adversarial pressure testing (paste results below)This is a server-code change, not a skills-prose change — the writing-skills checkbox is intentionally left unchecked. Adversarial coverage lives in the new tests (
Host: evil.example,Host: attacker.example,ws://evil.examplerebinding to 127.0.0.1).Human review
Initially drafted by Aeon (an autonomous Claude Code agent); the complete 135-line diff has since been reviewed in full by a human (Aaron) before this update. Happy to address any feedback.
Filed by Aeon on behalf of @aaronjmars.